Fix envTest topologyRef race condition #569
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fmount The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
fmount
force-pushed
the
toporef_envtest
branch
from
ebfb56c to
400e34a
Compare
fmount
changed the title
|
/test functional |
2 similar comments
|
/test functional |
|
/test functional |
fmount
force-pushed
the
toporef_envtest
branch
from
400e34a to
36ca107
Compare
|
/test functional |
7 similar comments
|
/test functional |
|
/test functional |
|
/test functional |
|
/test functional |
|
/test functional |
|
/test functional |
|
/test functional |
last failure is unrelated as Prow wasn't able to start the job. |
|
/test functional |
3 similar comments
|
/test functional |
|
/test functional |
|
/test functional |
fmount
force-pushed
the
toporef_envtest
branch
2 times, most recently
from
200f72e to
0de0624
Compare
|
/test functional |
2 similar comments
|
/test functional |
|
/test functional |
fmount
changed the title
|
/test functional |
1 similar comment
|
/test functional |
| finalizers := prevTopology.GetFinalizers() | ||
| g.Expect(finalizers).To(BeEmpty()) | ||
| }, timeout, interval).Should(Succeed()) | ||
| }, 20*timeout, interval).Should(Succeed()) |
There was a problem hiding this comment.
Do we need the 20x timeout here given that we think we know what is going on? I just wonder if you had added this strictly for testing purposes.
There was a problem hiding this comment.
Hey no, I think I simply forgot that value here! Good catch, updating the patch right away!
There was a problem hiding this comment.
This is weird, while locally it works after removing the 20 * timeout, in CI it fails.
Before this patch, the "updates topology" envTest was a single block of instructions, but we actually need to check each of them provided that the reconcile loop moves forward and certain conditions are satisfied. This patch attempts to split the Eventually block so that: - Each assertion gets its own retry logic (based on timeout, interval) - Prevents early assertion failures from masking later issues This approach has been applied already to other operators (e.g. Nova) where there are many controllers trying to apply/remove finalizers to the same resource. Signed-off-by: Francesco Pantano <[email protected]>
fmount
force-pushed
the
toporef_envtest
branch
from
0de0624 to
c7eb9d4
Compare
|
@fmount: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Before this patch, the
updates topologyenvTestwas a single block of instructions, but, provided that the reconcile loop moves forward and certain conditions are satisfied, we actually need to check each of them.This patch attempts to split the
Eventuallyblock so that:timeout,interval)This approach has been applied already to other operators (e.g.
Nova) where there are many controllers trying to apply/removefinalizersto/from the same resource.